feat(jedis): regenerate jedis-3.0 instrumentation [reference] - #12080
feat(jedis): regenerate jedis-3.0 instrumentation [reference]#12080jordan-wong wants to merge 2 commits into
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 567dc87 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
I updated the label to the ones from the repo. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 567dc872cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tag("_dd.peer.service.source", any()), | ||
| tag("_dd.svc_src", any()), | ||
| tag("peer.service", any()), | ||
| error(JedisDataException.class), |
There was a problem hiding this comment.
Match the exception message tag
When LPUSH throws the expected JedisDataException, BaseDecorator.onError calls DDSpan.addThrowable, which always emits error.msg; however, error(JedisDataException.class) only matches error.type and error.stack, so SpanMatcher rejects the extra message tag and this test fails in both forked variants. Capture the exception returned by assertThrows and pass it to error(exception), or explicitly match its message.
Useful? React with 👍 / 👎.
| tag("_dd.peer.service.source", any()), | ||
| tag("_dd.svc_src", any()), | ||
| tag("peer.service", any()), |
There was a problem hiding this comment.
Preserve exact peer-service assertions
For the v1 fork, the removed Groovy test's peerServiceFrom(Tags.PEER_HOSTNAME) verified that peer.service equals the hostname and that _dd.peer.service.source identifies that hostname tag. Replacing those checks with any() accepts incorrect values and even absent tags because SpanMatcher treats an unmatched Any matcher as optional, so a peer-service regression now passes the entire suite; match the expected values and schema-specific presence instead.
Useful? React with 👍 / 👎.
| @@ -1,133 +0,0 @@ | |||
| # This is a Gradle generated file for dependency locking. | |||
There was a problem hiding this comment.
Restore the module dependency lock
Deleting this lock file leaves the module's dependency graph—including the latestDepTest declaration using dynamic version 3.+—unpinned, so identical revisions can resolve different Jedis and transitive versions over time. The repository's Gradle source of truth requires a per-project gradle.lockfile; regenerate it after changing the test dependency rather than removing it.
AGENTS.md reference: AGENTS.md:L39-L44
Useful? React with 👍 / 👎.
|
|
||
| @WithConfig(key = "trace.span.attribute.schema", value = "v0") | ||
| @WithConfig(key = "trace.db.client.split-by-instance", value = "true") | ||
| class Jedis30ClientV0ForkedTest extends Jedis30ClientTest { |
There was a problem hiding this comment.
Restore latest-dependency test execution
The only concrete v0 test was changed from a regular test to a ForkedTest, but this module registers only latestDepTest, not latestDepForkedTest; forked classes are excluded from latestDepTest, and the remaining shared class is abstract. Consequently the task can report green without executing any instrumentation tests against the declared Jedis 3.+ dependency, so retain a non-forked v0 concrete class or register and run a latest-dependency forked suite.
AGENTS.md reference: AGENTS.md:L63-L65
Useful? React with 👍 / 👎.
🤖 Generated with APM Instrumentation Toolkit
[Reference PR — not intended to be merged as-is.] This is a toolkit-generated regeneration of the existing
jedis-3.0instrumentation, opened as a reference artifact to evaluate generation quality against the current instrumentation guidance. It is not a request to replace the production module.What Does This Do
Regenerates the
jedis-3.0Redis client instrumentation from scratch:redis.clients.jedis.Connection#sendCommand(ProtocolCommand, ...)to create a client span per Redis command, with a class-loader matcher scoping it to Jedis 3.x (ProtocolCommandpresent,CommandObjectabsent).fail [,3.0.0)+pass [3.0.0,4.0.0), includingskipVersions += "jedis-3.6.2"to handle that release's malformedjedis--prefixed version string.AbstractInstrumentationTest/SpanMatcher/TagsMatcher), with base + V0 + V1 forked variants.Motivation
Evaluates whether the current instrumentation guidance produces a correct, muzzle-clean Jedis module without manual intervention — in particular whether the muzzle version-range pattern and the
skipVersionslibrary quirk are handled automatically. Both were produced correctly this run (theskipVersionsgap was caught and fixed by an automated review check rather than requiring a manual CI round-trip).Additional Notes
Local CI-equivalent verification (
:check :muzzle :latestDepTest, single-JVM):muzzle— GREEN (all AssertPass/AssertFail tasks pass;jedis-3.6.2correctly skipped)spotless— GREENlatestDepTest— reported green but executes no concrete tests (see CI Triage Status below); not a meaningful pass.forkedTest— 2 red:wrongTypeCommandSetsErrorTags(V0 + V1). See the corrected root-cause analysis and the automated-review findings in CI Triage Status below. These are open items for maintainer decision, not yet resolved.Multi-JVM CI matrix (JDK 8/17/21/25): the same single shard (6/8 = jedis-3.0) fails identically across all JVMs.
Comparison against the existing
jedis-3.0modulesuper("jedis", "redis")): identical — no rename, no config-surface change.skipVersions += "jedis-3.6.2".metadata/supported-configurations.json: jedis entries identical — no dropped_ENABLEDflags.classLoaderMatcher()(scoping to Jedis 3.x by requiringProtocolCommandand excludingCommandObject) that the current module does not have — the current module relies on muzzle range alone for version scoping. Additive and arguably a tighter classloader isolation, but noting it explicitly for reviewers.JedisInstrumentation+JedisClientDecorator, no helper extraction) and the standard v0/v1 attribute-schema forked-test split.CI Triage Status
Last updated: 2026-07-28
Classified as infra / test-framework gap (not an instrumentation defect):
dd-gitlab/test_inst: [8|11|17|21|25|tip, 6/8]— all six JVM variants of shard 6/8 fail onwrongTypeCommandSetsErrorTags. Shard 6/8 is confirmed to be:dd-java-agent:instrumentation:jedis:jedis-3.0(verified via the repo'sabs(project.path.hashCode() % 8) + 1shard math). Reproduces identically in localforkedTest. Corrected root cause: the assertionUnexpected error status. True expected but was: false— the generated test asserts theLPUSHspan is marked as an error (from Redis'sWRONGTYPEreply), but the span is not. This is a generated test that the existingjedis-3.0module never had (master has no wrong-type/error test); Jedis 3.x surfaces theWRONGTYPEerror when the response is read, not as a throwable out of the instrumentedsendCommand, so the@Advice.Thrownat method exit is null andDECORATE.onErrornever marks the span. Needs a maintainer decision: either the error-capture hook point is genuinely narrower than the test assumes (instrumentation-logic finding) or the invented test over-asserts. Not silently fixed.dd-gitlab/default-pipeline— umbrella pipeline; fails only because the shard 6/8 child above fails. No independent cause.Confirmed findings from automated (Codex) review — do not silently fix:
latestDepTestruns zero concrete tests (Codex P2): both concrete test classes (Jedis30ClientV0ForkedTest,Jedis30ClientV1ForkedTest) are*ForkedTest, but the module registers onlylatestDepTest, notlatestDepForkedTest. Forked classes are excluded fromlatestDepTest, and the remaining shared class is abstract — solatestDepTestcan report green while executing no instrumentation tests against the declared Jedis3.+. This invalidates the earlier "latestDepTest GREEN" claim. Needs either a non-forked v0 concrete class or a registeredlatestDepForkedTestsuite.peer.serviceassertions weakened toany()(Codex P2): the removed Groovy test verifiedpeer.serviceequals the hostname and_dd.peer.service.sourceidentifies that tag; the generated test replaced these withany(), which passes even if the tag is absent or wrong — a silent loss of coverage vs. the original.error.msgnot matched (Codex P1):error(JedisDataException.class)matches onlyerror.type/error.stack, butBaseDecorator.onError → DDSpan.addThrowablealso emitserror.msg; the strict matcher would reject the extra tag. (Secondary to the error-status failure above, but same test.)Fixed (config/mechanical):
spotless— formatting drift from the generation run, fixed viaspotlessApply(commit567dc872cc) before the PR was opened. Currently green.To fix (mechanical, pending push approval):
gradle.lockfile(Codex P2): AGENTS.md requires a per-projectgradle.lockfile; the regeneration deleted it, unpinning the dependency graph (including the3.+dynamiclatestDepTestversion). Should be regenerated, not removed. Mechanical (a)-class fix.Still unclassified: none.
Pre-review checklist
jedis-3.0; siblingsjedis-1.4/jedis-4.0untouched; draft skill confirmed loaded.jedis/jedis-3.0/, no.analysis/pollution, no sibling contamination.classLoaderMatcher(), see above).super(...), muzzle range, and metadata all preserved verbatim.Thread.sleep, nojvmArgsoverride, no helper-class extraction; forked variants isolate on@WithConfig(trace.span.attribute.schema)which is self-documenting.latestDepTestreported green but — per Codex review — executes no concrete tests (see findings above), so it is not a meaningful pass.forkedTestred on the error-status assertion (see corrected root cause above). Same single shard fails across all JVMs.gradle.lockfile); 3 review findings documented and left un-fixed for maintainer decision (error-status test, zero-concrete-latestDep, weakened peer.service assertions).Jira ticket: [N/A — reference artifact]
🤖 Generated with APM Instrumentation Toolkit